Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Peers need to check each other's dust limit #894

Merged
merged 4 commits into from
Oct 6, 2021
Merged

Conversation

t-bast
Copy link
Collaborator

@t-bast t-bast commented Aug 11, 2021

Since HTLCs below this amount will not appear in the commitment tx, they are effectively converted to miner fees. The peer could use this to grief you by broadcasting its commitment once it contains a lot of dust HTLCs.

I just stumbled upon issue #696 which mentioned that a while ago, but we didn't update the spec accordingly.
However, implementations do already enforce that.

I also added a lower bound on dust_limit_satoshis that ensures that transactions will correctly propagate with default relay policies.

Fixes #696 and #905

@@ -239,6 +239,7 @@ The receiving node MAY fail the channel if:
- it considers `channel_reserve_satoshis` too large.
- it considers `max_accepted_htlcs` too small.
- it considers `dust_limit_satoshis` too small and plans to rely on the sending node publishing its commitment transaction in the event of a data loss (see [message-retransmission](02-peer-protocol.md#message-retransmission)).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we drop this? I believe c-lightning and lnd both don't implement this at all today, and I'm not sure if we need to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's still useful, and I kept a lower bound on eclair, because you may want to avoid letting your peer create a commit tx that you think will not easily propagate through the bitcoin network because it doesn't satisfy default policy settings.

If you think you'll rely on their commit tx with option_dataloss_protect, you want to be sure their commit tx can get to miners' mempools, don't you?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think you'll rely on their commit tx with option_dataloss_protect, you want to be sure their commit tx can get to miners' mempools, don't you?

Yea, that's a good point. Should we clarify that a suggested lower-bound is 330 (which IIRC is the exact current network dust threshold)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in f16ea99

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: I instead added a list of the network dust threshold in 40cfede which implementers can refer to when deciding what lower bound they want to enforce.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's really sufficient - there needs to be guidance in what implementations should be allowed to set the dust limit to, as otherwise we end up back where we are today where some nodes arbitrarily force-closed based on dust limit and there isn't a consensus. I'd strongly prefer if we either agree to set it to the max value for a standard output, or we set it to 330 and then fix the close negotiation logic via #896.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I agree with that. Both options have pros and cons:

Note that an alternative to #896 which would also fix the issue would be to disallow non-segwit scripts in shutdown. Now that segwit is widely supported, this would fix a few headaches.

I'd like feedback from other implementers on this choice, @rustyrussell @Roasbeef what do you think?

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Aug 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, if we can get agreement on it, I'd love to just say "segwit-only shutdown", drop 896 and then set dust limit to be the segwit lower bound (354, I think? check my math). Do any implementations in practice do non-segwit closing scripts today? It would mean you can't open a channel with that output which seems incredibly restrictive.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can close to cold storage, I guess, which may be old skool. But simplicity FTW: you can always unilaterally close so I've never considered it a big deal whatever we do here.

IOW, I'm happy with adding "mutual quickclose must be segwit" in #847

Copy link
Collaborator Author

@t-bast t-bast Aug 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened an issue to discuss the dust_limit_satoshis lower bounds in #905

I'd like to treat this in a separate PR than this one, which fixes a different issue that we should have fixed a long time ago (adding a higher bound on our peer's dust_limit_satoshis).

If my calculations of the various dust thresholds imposed by Bitcoin Core are correct, I believe we can merge this PR as the need to impose a higher bound on remote dust should be obvious to everyone?

@t-bast
Copy link
Collaborator Author

t-bast commented Aug 16, 2021

There can be issues with dust limits below 546, as pointed out here.

This is also a concern with future segwit versions, for which we don't know the bitcoin network dust threshold (when using option_shutdown_anysegwit). I believe we need a new mechanism to handle that kind of issues.

@TheBlueMatt
Copy link
Collaborator

There can be issues with dust limits below 546, as pointed out here.

That's a good point, I'd like to hear from lnd and c-lightning folks whether they think this matters given they currently don't enforce this at all. I presume they force-close channels if the closing transaction doesn't confirm in some time?

This is also a concern with future segwit versions, for which we don't know the bitcoin network dust threshold (when using option_shutdown_anysegwit). I believe we need a new mechanism to handle that kind of issues.

Right, this seems to be a general problem - the only thing you can realistically do here is force-close if nothing confirms in some time because at least you know all the scripts used in that transaction.

@t-bast
Copy link
Collaborator Author

t-bast commented Aug 16, 2021

AFAICT the only place where peers are free to use different types of scripts is during mutual close.

The spec could list in the mutual close requirements the dust threshold for each type of script (e.g. 546 sats for non-segwit, 330 sats for segwit, ??? for taproot) and both nodes should enforce these static checks and prune outputs accordingly. If one of the peers doesn't, this will automatically result in a force-close since the closing signatures won't match.

@TheBlueMatt
Copy link
Collaborator

??? for taproot

I tried to ask about this on #bitcoin-core-dev and basically got told to fuck off and not rely on relay dust limits for anything, so I guess for now just assume it won't change cause Core doesn't change things and hope for the best.

The spec could list in the mutual close requirements the dust threshold for each type of script (e.g. 546 sats for non-segwit, 330 sats for segwit, ??? for taproot) and both nodes should enforce these static checks and prune outputs accordingly. If one of the peers doesn't, this will automatically result in a force-close since the closing signatures won't match.

That is an incompatibility, I think - one issue with closing_signed today is that it talks about how nodes can unilaterally drop their own output (which would be required for this to work), but then talks about how if the fee_satoshis field is the same as the one it sent it's done, even though the transaction may be different. Maybe we should fix it in #847?

If we fix the spec to actually allow unilateral dropping of a node's output, we could reasonably just say that nodes can send a dust threshold down to 330, but have to enforce that counterparties have removed their output during closing if its lower than the scripts real dust threshold.

@Crypt-iQ
Copy link
Contributor

In case anybody was wondering, I believe p2tr outputs will have a dust limit defined by Core's GetDustThreshold func as 330 sats, which is the same as p2wsh outputs. It may be a good idea to place the different output type's dust values calculated at dust_relay_fee of 3000sat/kvb somewhere?

Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ⛷

02-peer-protocol.md Outdated Show resolved Hide resolved
@t-bast
Copy link
Collaborator Author

t-bast commented Sep 14, 2021

As discussed during yesterday's spec meeting, I added option 2 of #905 in 3e7771e

  • we explicitly reject dust limits below 354 sats to ensure there won't be propagation issues with default relay policies
  • we drop support for non-segwit scripts in shutdown
  • if at closing time we realize that an output will prevent propagation, we force-close

@TheBlueMatt
Copy link
Collaborator

Looks good to me. Is it worth mentioning in the rationale section that older nodes will send non-segwit shutdown scripts and you may wish to allow them for backwards-compatibility today, taking note of the force-close requirement at shutdown.

Since HTLCs below this amount will not appear in the commitment tx, they
are effectively converted to miner fees. The peer could use this to grief
you by broadcasting its commitment once it contains a lot of dust HTLCs.

Fixes #696
As implemented in Bitcoin Core's default relay policy.
This allows dust limit to go as low as 354 sats without creating relay
issues with default node policies.

We add a requirement that dust limit cannot be lower than 354 sats.
This ensures implementers don't have to figure this subtlety on their own.
@t-bast
Copy link
Collaborator Author

t-bast commented Sep 15, 2021

Is it worth mentioning in the rationale section that older nodes will send non-segwit shutdown scripts and you may wish to allow them for backwards-compatibility today, taking note of the force-close requirement at shutdown.

You're right, it's hard to understand without any context. I rebased and added some rationale in 9ff1387

t-bast added a commit to ACINQ/eclair that referenced this pull request Sep 15, 2021
We are slowly dropping support for non-segwit outputs, as proposed in
lightning/bolts#894

We can thus safely allow dust limits all the way down to 354 satoshis.
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Sep 15, 2021
We are slowly dropping support for non-segwit outputs, as proposed in
lightning/bolts#894

We can thus safely allow dust limits all the way down to 354 satoshis.

In very rare cases where dust_limit_satoshis is negotiated to a low value,
our peer may generate closing txs that will not correctly relay on the
bitcoin network due to dust relay policies.

When that happens, we detect it and force-close instead of completing the
mutual close flow.
03-transactions.md Outdated Show resolved Hide resolved
03-transactions.md Outdated Show resolved Hide resolved
03-transactions.md Outdated Show resolved Hide resolved
@@ -270,6 +269,7 @@ The receiving node MUST fail the channel if:
- `funding_pubkey`, `revocation_basepoint`, `htlc_basepoint`, `payment_basepoint`, or `delayed_payment_basepoint`
are not valid secp256k1 pubkeys in compressed format.
- `dust_limit_satoshis` is greater than `channel_reserve_satoshis`.
- `dust_limit_satoshis` is smaller than `354 satoshis` (see [BOLT 3](03-transactions.md#dust-limits)).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should only apply for option_shutdown_anysegwit and otherwise allow a lower bound of 330 satoshis.

Copy link
Contributor

@Crypt-iQ Crypt-iQ Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correction: I see eclair's reasoning for not allowing lower than 546 unless segwit-only shutdown. Maybe this should only apply if the node has option_shutdown_anysegwit & segwit-only shutdown. I know the latter isn't a feature-bit, but it could be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should only apply for option_shutdown_anysegwit and otherwise allow a lower bound of 330 satoshis.

I disagree, because option_shutdown_anysegwit can be activated after the channel is opened.
For example, Alice and Bob open a channel without option_shutdown_anysegwit and set dust_limit to 330 sats.
Then later they both activate option_shutdown_anysegwit (because why not?).
If they didn't use an upfront_shutdown_script, they're allowed to use a segwit v1+ script when closing the channel, which can be an issue for dust limits below 354 sats.
So let's be safe and use 354 sats which is future-proof.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, that makes sense.

t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Sep 23, 2021
We are slowly dropping support for non-segwit outputs, as proposed in
lightning/bolts#894

We can thus safely allow dust limits all the way down to 354 satoshis.

In very rare cases where dust_limit_satoshis is negotiated to a low value,
our peer may generate closing txs that will not correctly relay on the
bitcoin network due to dust relay policies.

When that happens, we detect it and force-close instead of completing the
mutual close flow.
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check all the dust limits, but I believe you :)

t-bast added a commit to ACINQ/eclair that referenced this pull request Sep 28, 2021
We are slowly dropping support for non-segwit outputs, as proposed in
lightning/bolts#894

We can thus safely allow dust limits all the way down to 354 satoshis.
t-bast added a commit to ACINQ/eclair that referenced this pull request Sep 29, 2021
We are slowly dropping support for non-segwit outputs, as proposed in
lightning/bolts#894

We can thus safely allow dust limits all the way down to 354 satoshis.

In very rare cases where dust_limit_satoshis is negotiated to a low value,
our peer may generate closing txs that will not correctly relay on the
bitcoin network due to dust relay policies.

When that happens, we detect it and force-close instead of completing the
mutual close flow.
@t-bast
Copy link
Collaborator Author

t-bast commented Oct 6, 2021

Merging this PR as discussed during the last spec meetings and explicitly approved since.

@t-bast t-bast merged commit 8f2104e into master Oct 6, 2021
@t-bast t-bast deleted the check-remote-dust-limit branch October 6, 2021 07:40
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Oct 20, 2021
We are slowly dropping support for non-segwit outputs, as proposed in
lightning/bolts#894

We can thus safely allow dust limits all the way down to 354 satoshis.

In very rare cases where dust_limit_satoshis is negotiated to a low value,
our peer may generate closing txs that will not correctly relay on the
bitcoin network due to dust relay policies.

When that happens, we detect it and force-close instead of completing the
mutual close flow.
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Oct 20, 2021
We are slowly dropping support for non-segwit outputs, as proposed in
lightning/bolts#894

We can thus safely allow dust limits all the way down to 354 satoshis.

In very rare cases where dust_limit_satoshis is negotiated to a low value,
our peer may generate closing txs that will not correctly relay on the
bitcoin network due to dust relay policies.

When that happens, we detect it and force-close instead of completing the
mutual close flow.
t-bast added a commit to ACINQ/lightning-kmp that referenced this pull request Oct 20, 2021
We are slowly dropping support for non-segwit outputs, as proposed in
lightning/bolts#894

We can thus safely allow dust limits all the way down to 354 satoshis.

In very rare cases where dust_limit_satoshis is negotiated to a low value,
our peer may generate closing txs that will not correctly relay on the
bitcoin network due to dust relay policies.

When that happens, we detect it and force-close instead of completing the
mutual close flow.
niftynei added a commit to niftynei/lightning-rfc that referenced this pull request Jan 31, 2022
P2SH outputs have larger dust requirements than native SegWit (and
Taproot) scripts; we disallow the creation of them on funding txs as a
they're problematic for relay when allowing dust limits below 546 sats
(see lightning#894)

Suggested-by: @t-bast
niftynei added a commit to niftynei/lightning-rfc that referenced this pull request Aug 3, 2022
P2SH outputs have larger dust requirements than native SegWit (and
Taproot) scripts; we disallow the creation of them on funding txs as a
they're problematic for relay when allowing dust limits below 546 sats
(see lightning#894)

Suggested-by: @t-bast
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BOLT 2: peers need to check each other's dust limit
5 participants